Reach Slack, Teams and PagerDuty without something in between - #33
Merged
Conversation
The webhook sink's own remarks claimed it was "the one sink worth shipping, because it is the one that reaches everything else", and listed Slack, Teams and PagerDuty among the things it reached. It does not reach any of them. All three reject arbitrary JSON: a Slack incoming webhook wants text and attachments or blocks, Teams wants an Adaptive Card in a Bot Framework envelope, and PagerDuty wants an Events API event. Posting the generic payload at a Slack webhook returns invalid_payload. The remarks half admitted it, in the phrase "Teams through a Power Automate flow" -- the flow existed to reshape the payload, which is work this library was leaving to its users. Three sinks, in the alerting package rather than a package each. That is the one place this deviates from what the roadmap said: it called for packages, and a package buys isolation of dependencies. These have none to isolate -- every one is HttpClient and System.Text.Json, both already here -- so three more package ids, readmes, and release entries would cost real maintenance and give nothing back. PagerDuty is the one that is not just a payload shape. It does not want to be told a health changed; it wants to be told an incident opened or closed, and which one. Alert.DeduplicationKey and Alert.IsRecovery already existed for exactly that, with a comment saying a flapping checker is one incident rather than several -- so a failure triggers keyed on the checker and a recovery resolves that same incident instead of raising a second one saying everything is fine. Suspicious maps to warning rather than critical, because it is the state that means "failing, but not past the threshold yet" and paging on it would defeat having a threshold. Teams targets the Workflows URL and an Adaptive Card. The Office 365 connectors and the MessageCard payload they took are retired, so a sink written to that shape would have arrived already dead. The payload shapes are asserted against the request body rather than against the records that build them, because the failure mode is a field named right in C# and wrong in JSON, which shows up only at the far end where no test would see it. The webhook sink had no coverage at all and now shares the same harness.
ASingleDueOccurrence_TriggersOnceEvenIfTicksKeepComing scheduled a checker on a one-millisecond period and then asserted that a second tick would not fire it. With that period the second tick is legitimately due after a single millisecond, so the test failed whenever anything put a millisecond between the two calls -- which says nothing at all about whether one occurrence can fire twice, the thing the test is named for. Its own comment described the arrangement it wanted, "due immediately, then not again for an hour", and the code underneath did not build that. The period is half a second now, and the wait is for that one occurrence to come due. The two ticks then have 500ms of slack between them rather than one. This is pre-existing and unrelated to the sinks; it is here because adding a test class changed how xUnit schedules the collections and made it fail about half the time on net10.0, which would otherwise have looked like the sinks breaking something. On main it fails roughly one run in two in isolation, and it is almost certainly the one unexplained red run earlier in this branch's history. Still catches what it is for: with the scheduler mutated to leave a consumed occurrence due, it goes red.
Review, checking the payloads against the services' own documentation rather than against these tests, found three things. The Adaptive Card colours were PascalCase. The schema's Colors enum is lowercase and matching is case-sensitive, and an unrecognised value is not an error: the card renders in the default colour and the POST still returns 2xx. Every Unhealthy and Suspicious alert would have lost its colour in Teams and nothing would ever have said so. The test asserted "Attention", so it pinned the bug rather than catching it. Confirmed against the published schema, which lists default|dark|light|accent|good|warning|attention. Slack parses the top-level text as mrkdwn, so &, < and > have to be escaped there. DisplayName is virtual and "Auth & Session" is an ordinary thing to call a checker. The attachment's fields are plain text unless mrkdwn_in says otherwise, and it does not, so only the headline needed it. PagerDuty's group was the one nullable field here without JsonIgnore(WhenWritingNull), so a checker with no group -- the common case -- sent "group": null while every other absent field was omitted. The UTC test was vacuous. It built its alert with DateTimeKind.Utc, and DateTimeOffset takes its offset from the Kind, so deleting the SpecifyKind call it was named after produced byte-identical output. It now uses an unmarked time, which is what a state round-tripped through a store that does not preserve Kind gives you, and which is the only case that tells the two apart. The Coravel test is deterministic rather than merely less flaky. Widening the window from one millisecond to five hundred made the failure rarer without changing its nature -- the second tick still raced the wall clock, and a long enough stall would still lose. The scheduler takes a TimeProvider now, optional so nothing about constructing it changes, and the test moves the clock by hand instead of sleeping. It asserts the next occurrence still arrives, which sleeping never did, and the suite spends 550ms less doing it.
ivanvyd
force-pushed
the
feature/alert-sinks
branch
from
July 29, 2026 19:42
dadca0d to
14d1eae
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The last item on the README roadmap: "Alert sinks beyond the webhook — Slack, Teams and PagerDuty."
The webhook did not reach them
WebhookAlertSink's own remarks claimed it was "the one sink worth shipping, because it is the one that reaches everything else", and listed Slack, Teams and PagerDuty among the things it reached. It reaches none of them. All three reject arbitrary JSON:text, plus attachments or blocksinvalid_payloadrouting_keyThe remarks half-admitted it, in the phrase "Teams through a Power Automate flow" — that flow existed purely to reshape the payload, which is work the library was leaving to its users. Those remarks are corrected here.
One deviation from the roadmap
The roadmap said "as packages". These are three sinks inside
Healthie.NET.Alertinginstead.A separate package buys isolation of dependencies. These have none to isolate — every one is
HttpClientplusSystem.Text.Json, both already referenced by the alerting package. Three more package IDs, READMEs, and release entries would be real ongoing maintenance in exchange for nothing. Say the word and I'll split them.PagerDuty is more than a payload shape
It does not want to be told a health changed; it wants to be told an incident opened or closed, and which one. That machinery already existed and was unused:
Alert.DeduplicationKey— documented as "a checker that flaps between suspicious and unhealthy is one incident, not two" — becomesdedup_key.Alert.IsRecovery— documented as "a recovery closes an incident rather than opening one, and a sink that treats the two alike pages somebody at three in the morning to tell them everything is fine" — becomesevent_action: resolverather than a secondtrigger.Suspiciousmaps towarning, notcritical: it is the state meaning "failing, but not past the threshold yet", and paging on it would defeat having a threshold at all.Teams targets Workflows, not connectors
Microsoft has retired Office 365 connectors and the
MessageCardpayload they accepted. A sink written to that shape would have shipped already dead, so this posts an Adaptive Card to the Workflows URL.Verification
383 tests green on both net8.0 and net10.0.
Payloads are asserted against the request body, not against the records that build them — the failure mode here is a field named right in C# and wrong in JSON, which shows up only at the far end where no test would see it.
WebhookAlertSinkhad no coverage at all and now shares the same harness.Mutation-tested:
Second commit: a pre-existing flaky test
Adding a test class changed how xUnit schedules collections and made
CoravelSchedulerTests.ASingleDueOccurrence_TriggersOnceEvenIfTicksKeepComingfail about half the time on net10.0 — which would otherwise have looked like the sinks breaking something.It is not the sinks, and it is not the scheduler. The test scheduled a 1 ms period and then asserted a second tick would not fire — but with a 1 ms period the second tick is legitimately due after one millisecond. It failed whenever anything put a millisecond between the two calls. Its own comment described the arrangement it wanted ("due immediately, then not again for an hour") and the code underneath did not build it.
Confirmed pre-existing: on
main, in isolation, it fails ~3 runs in 6 on net10.0. It is almost certainly the one unexplained red run earlier in this session that I could not reproduce at the time. Period is now 500 ms, giving the two ticks 500 ms of slack instead of one; 8/8 stable after. Still catches what it is named for — with the scheduler mutated to leave a consumed occurrence due, it goes red.This also affects PRs #30, #31 and #32, which share the same suite.
No release — version stays at 3.1.4.